review-pr: Avoid OIDC for direct API key reviews - #40

Review — head b78047f

CI: ✅ Lint, Typecheck, Unit Tests, Integration Tests all success. Note these results are for the current head, which no longer merges cleanly with main, so they don't cover the post-rebase state.
Mergeable: ❌ CONFLICTING.

The direction is right and the TypeScript side is genuinely well covered. Three things block merging.

Blocking

1. [blocking] Conflicts with main. git merge-tree origin/main pull/40/head → CONFLICT (content) in .github/workflows/review-pr.yml and review-pr/README.md. Merge-base is ddb0139; main has since moved internal pins to v2.0.4, added the Check trigger context exists step, timeout-minutes: 60, and actions: write.

2. [blocking] The actions: write requirement and its docs get lost / contradicted. main review-pr.yml:293 has actions: write # cache delete for review-lock release cleanup; feedback artifact list/delete, documented as required since v2.0.3 in README.md:238,242 and review-pr/README.md:10,40,121. This PR removes every permissions: block, and its review-pr/README.md (head lines 37, 115, 130) tells callers actions: read. On merge, README.md auto-merges (keeping "actions: write required since v2.0.3") while review-pr/README.md conflicts — so the resolved docs will contradict each other and the workflow declares nothing at all. Two consequences:

• (a) A caller following the new text grants actions: read; review-lock cache cleanup and feedback-artifact list/delete then 403 silently at runtime, where previously that misconfiguration failed loudly at startup validation.

• (b) The caller-permissions release safeguard (issue #72) goes inert. Demonstrated with the repo's own extractor (computeCallerRequirement(parseWorkflowPermissions(...))):

  • main → {"contents":"read","pull-requests":"write","issues":"write","id-token":"write","actions":"write","checks":"write"}
  • this PR → {}

  so diffCallerRequirements can never flag a future increase for review-pr.yml again.

Dropping the block is the only way to make id-token optional (a called workflow can't request permissions conditionally), so the fix isn't to restore it — but the docs must keep stating actions: write and what degrades without it, say that id-token: write is only for the AWS path, and the safeguard needs a declared requirement source it can still read.

3. [blocking] Internal uses: pins point at a PR-branch commit with a non-version comment. review-pr.yml:296, 571, 805, 873 → @5e46aee485641c251fdaeb9a05bd9977c711def8 # direct API key OIDC fix. 5e46aee is this PR's own first commit and is not on main; main pins the same action at b08367e… # v2.0.4 at 6 sites, and no workflow/script in this repo rewrites internal pins. As-is this ships a reusable workflow resolving setup-credentials and mention-reply from an unreleased branch commit, mixed with v2.0.2/v2.0.4 pins in the same file. Repoint to the released SHA + version comment (AGENTS.md, "Versioning & releases").

Should fix

4. [should-fix] The auth-model downgrade is inferred from an empty env var rather than stated. src/check-org-membership/index.ts:294 dispatches on inputs.orgToken && inputs.org, and main() no longer fails when ORG_MEMBERSHIP_TOKEN is absent (the if (!orgToken) core.setFailed(...) guard is gone, index.ts:346). setup-credentials/action.yml only verifies GITHUB_APP_TOKEN, never ORG_MEMBERSHIP_TOKEN. So an OIDC-path run whose secret payload loses ORG_MEMBERSHIP_TOKEN (or whose ORG env is unset) silently switches from "Docker org member" to "has repo write" instead of failing closed. Pass the intent explicitly (an AUTH_MODE mirroring fetch-credentials) and keep the hard failure when org mode is expected.

5. [should-fix] A direct key silently wins over OIDC even when the caller wants OIDC. review-pr.yml:174 derives HAS_DIRECT_API_KEY from any non-empty model-key secret, and :296 then passes fetch-credentials: false. A caller granting id-token: write that also inherits, say, an org-level ANTHROPIC_API_KEY gets no GITHUB_APP_TOKEN — so the bot posts as github-actions[bot] instead of docker-agent (:409 github-token: ${{ env.GITHUB_APP_TOKEN || github.token }}) — plus the weaker authorization path. That contradicts the wording this PR adds ("Preferred: OIDC authentication to AWS Secrets Manager", review-pr/README.md:114). An explicit input (credentials: auto|aws|direct) or OIDC-wins precedence would be predictable.

6. [should-fix] New authorization logic added as inline bash, duplicating the new TS function. review-pr.yml:604-611 reimplements checkRepositoryWritePermission (src/check-org-membership/index.ts:120-138). AGENTS.md ("TypeScript / src rules") requires new composite-action logic in src/ with Vitest tests; the bash copy is untested and its admin|maintain|write allowlist must be kept in sync by hand. It also behaves differently: ... || echo "none" (:604) collapses any API error into "not authorized" and tells the user "does not have write access", while the TS path rethrows non-404s and fails loudly. Invoke dist/check-org-membership.js, as the review job already does at :321.

7. [should-fix] The new path has no end-to-end coverage. self-review-pr.yml passes no secrets: to the reusable workflow, so HAS_DIRECT_API_KEY is always false in dogfooding, and test-e2e-reviewer.yml is dispatch-only — nothing exercises the YAML gating. The unit tests cover the TS branch well (checkRepositoryWritePermission ×3, evaluateMembership fallback ×3, mention-reply fallback ×3); please add a dispatch E2E scenario, or a tests/*.sh for the new bash branch per repo convention.

8. [should-fix] Docs currency: AGENTS.md is untouched. It still describes setup-credentials/ as an unconditional OIDC fetch and check-org-membership/ as org-membership-only; the new fetch-credentials input and the repo-write authorization tier are absent from both the layout entries and the security-first authorization tiers.

9. [should-fix] The PR description is truncated mid-sentence ("The HAS_DIRECT_API_KEY guard that silently dis"), so the rationale for a security-relevant guard is missing from the durable record. The only linked issue is a Slack permalink — please add a repo-qualified GitHub issue for provenance.

Optional

10. [optional] review-pr/README.md:121-122 — "…you may omit id-token: write:" ends with a colon introducing the YAML block, but an unrelated "actions: read is always required…" sentence is spliced in between. Reorder so the colon leads into the example.

Verified as safe — no action needed

• Every GITHUB_APP_TOKEN consumer now has a || github.token fallback (:344, 409, 740, 816, 878, 900, 907); no step is left with an empty token when the OIDC fetch is skipped.
• Skipping setup-credentials in the reply jobs is safe: $DOCKER_AGENT_ACTION_ROOT is used only in the review job (:321, :346), which always runs the action, and the mention-reply handler is a node24 JS action needing no setup-node.
• checkRepositoryWritePermission fails closed (404 → false, other statuses rethrown); public-repo non-collaborators return read, not 404, and are correctly rejected.
• ?? → || for orgToken in src/mention-reply/index.ts:264 correctly stops an empty env var from winning.
• actionlint accepts the new env-in-with/if and secrets-in-job-env usages (only pre-existing SC2086 info remains).

Recommendation

Not mergeable as-is. Rebase onto main, settle the actions: write / docs question (2), and repoint the pins (3). Happy to re-review after that.